chore(shared-data): create liquid class schema v1 and fixture #16267

ncdiehl11 · 2024-09-17T16:28:45Z

Overview

This PR introduces the first version of an Opentrons liquid class schema. Single liquid class definitions will adhere to this schema and be keyed by pipette and tip type.

Closes AUTH-832

Test Plan and Hands on Testing

Look through schema and ensure it adheres to our finalized liquid class testing matrix. Any feedback on schema structure/style is appreciated.
Try out some test data in a JSON validator. I created a base fixture that passes schema validation. We can modify that for testing.

Changelog

create liquid class schema v1
add fixture

Review requests

See test plan. Should we leave keys for patternProperties pipette and tip type less restrictive strings? Perhaps any safeString rather than following more stringent regex?

Risk assessment

low

This PR introduces the first version of an Opentrons liquid class schema. Single liquid class definitions will adhere to this schema and be keyed by pipette and tip type.

sfoster1

I'm a little concerned with the regexes that you specify for tips and for pipettes. Because these are keys, they're tough to document; and because we want to keep this open for more pipettes and tips, it's tough to make a properly restrictive and self-documenting regex (insofar as regexes can ever be self-documenting).

What about instead having these levels be arrays, something like

"properties: {
    "liquidName": ...,
    ...,
    "byPipette": {
        "type": "array",
        "description": "The settings for each kind of pipette for this liquid",
       "items": {
             "type": "object",
             "description": "The settings for a specific kind of pipette when handling this liquid",
             "properties": {
                   "pipetteModel": {
                         "type": "string",
                          "description": "The pipette model this applies to",
                          // free to have an enum or a pattern or just a string here
                    },
                    "byTipClass": {
                           "description": "Settings for each kind of tip this pipette can use",
                           "type": "array",
                           "items": {
                                "type": "object",
                                "properties": {
                                     "tipClass": {
                                          // again, free to have different enums or patterns or whatever
                                     },
                                     ...
                                }
                           }
                    }
             }
        }
    }

sfoster1 · 2024-09-17T18:13:52Z

shared-data/liquid-class/schemas/1.json

+    }
+  },
+  "patternProperties": {
+    "^p[0-9]{2,4}": {


I see where you're going with the patternProperties here but I think it's going to be tough to come up with an appropriate regex and I'm not sure this is usefully it.

Yeah, that's a good callout. I like the idea of nesting the properties in arrays of objects containing the specific pipette/tip properties. Just pushed a commit to that effect and made some other refactors for only requiring certain properties if enable is true.

sfoster1 · 2024-09-17T18:13:59Z

shared-data/liquid-class/schemas/1.json

+      "type": "object",
+      "description": "Object containing liquid class properties specific to the pipette model",
+      "patternProperties": {
+        "^t[0-9]{2,4}$": {


same comment as with the pipette type

codecov · 2024-09-17T21:12:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.62%. Comparing base (a0f42de) to head (ea481a4).
Report is 23 commits behind head on edge.

Additional details and impacted files

@@           Coverage Diff           @@
##             edge   #16267   +/-   ##
=======================================
  Coverage   73.62%   73.62%           
=======================================
  Files          41       41           
  Lines        2992     2992           
=======================================
  Hits         2203     2203           
  Misses        789      789

Flag	Coverage Δ
shared-data	`73.62% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

sfoster1

Looks great to me! Sorry it took me a while to re-review.

sanni-t

This is awesome @ncdiehl! Just a couple of nitpicks.

We also need to add volume corrections but we can add those fields later

sanni-t · 2024-10-02T20:28:53Z

shared-data/liquid-class/schemas/1.json

+    "touchTip": {
+      "type": "object",
+      "description": "Shared properties for the touch-tip function.",
+      "properties": {
+        "enable": {
+          "type": "boolean",
+          "description": "Whether touch-tip is enabled."
+        },
+        "zOffset": {
+          "type": "number",
+          "description": "Offset from the top of the well for touch-tip, in millimeters."
+        },
+        "mmToEdge": {
+          "type": "number",
+          "description": "Offset away from the the well edge, in millimeters."
+        },
+        "speed": {
+          "$ref": "#/definitions/positiveNumber",
+          "description": "Touch-tip speed, in millimeters per second."
+        }
+      },
+      "if": {
+        "properties": {
+          "enable": { "const": true }
+        }
+      },
+      "then": {
+        "required": ["enable", "zOffset", "mmToEdge", "speed"]
+      },
+      "else": {
+        "required": ["enable"]
+      }
+    },


This works, but I think a more concise way of writing the required fields would be something like this-

"touchTip": { "type": "object", "description": "Shared properties for the touch-tip function.", "properties": { "enable": { "type": "boolean", "description": "Whether touch-tip is enabled." }, "zOffset": { "type": "number", "description": "Offset from the top of the well for touch-tip, in millimeters." }, "mmToEdge": { "type": "number", "description": "Offset away from the the well edge, in millimeters." }, "speed": { "$ref": "#/definitions/positiveNumber", "description": "Touch-tip speed, in millimeters per second." } }, "required": ["enable"], "if": { "properties": { "enable": { "const": true } } }, "then": { "required": ["zOffset", "mmToEdge", "speed"] } },

i.e., enable is required always. And if enable is true, require the additional properties

Same for all the other properties that have gating behind the enable key.

I'm going to go further than this and ask if there's a reason why we don't just get rid of enable and make the the availability of a particular liquid class property depend on it being defined for a particular tip type, i.e. if it's there it's enabled and if it's not defined it's not enabled. This would simplify the Python models and we could always use known defaults if a user wishes to enable a default disabled property.

This would simplify the Python models and we could always use known defaults if a user wishes to enable a default disabled property.

@jbleon95 while I agree that not having enable/disable makes the python models as well as this schema much more compact, using some known defaults if a user enables a property whose sub-properties aren't defined isn't a great solution. It would lead to confusion and uncertainty about what the defaults are for each of the enable-gated properties.

That said, maybe there's a middle ground. The schema doesn't include whether to enable or disable those properties but you specify it when using the transfer function. So essentially it'll behave the same way you described, except, the liquid class definition will contain definitions of all properties & sub-properties so that when a transfer function wants to use it, it uses the values from liquid class's definition instead of some defaults.

sanni-t · 2024-10-02T20:32:52Z

shared-data/liquid-class/schemas/1.json

+    },
+    "mix": {
+      "type": "object",
+      "description": "Mixing properties.",


Can we add a more elaborate description like you've used for submerge?

sanni-t · 2024-10-02T20:33:15Z

shared-data/liquid-class/schemas/1.json

+    },
+    "blowout": {
+      "type": "object",
+      "description": "Blowout properties.",


Same comment as mix description

sanni-t

Some nitpicks but mainly want to make sure that the 'by volume' properties are as discussed. We want them to be objects where keys are volumes. So something like-

xyzByVolume: {
    "default": 123.
    "100": 1.2,
    .
    .
}

"default" will be required for all.

The schema for it would be something like-

"airGapByVolume": {
      "type": "object",
      "description": "Settings for air gap keyed by target aspiration volume.",
      "items": {
        "$comment": "Other keys in here should be volume in uL",
        "properties": {
           "default": { 
              "$ref": "#/definitions/positiveNumber"}
        },
        "required": ["default"],
        "additionalProperties": { "$ref": "#/definitions/positiveNumber", }
      }
    },

You can even assign pattern properties to the additional keys I think. So they'll always be strings of numbers

shared-data/liquid-class/schemas/1.json

shared-data/liquid-class/fixtures/fixture_glycerol50.json

sanni-t

Changes look good!! Thanks!!

chore(shared-data): create liquid class schema v1 and fixture

ab3fa23

This PR introduces the first version of an Opentrons liquid class schema. Single liquid class definitions will adhere to this schema and be keyed by pipette and tip type.

ncdiehl11 self-assigned this Sep 17, 2024

add patternProperties descriptions for pipette and tip

19426dc

ncdiehl11 requested review from andySigler, sfoster1, shlokamin, sanni-t and jbleon95 September 17, 2024 16:36

sfoster1 requested changes Sep 17, 2024

View reviewed changes

ncdiehl11 added 2 commits September 17, 2024 17:05

refactor schema per feedback

ea481a4

add tipType property as required

e914bbc

ncdiehl11 marked this pull request as ready for review September 17, 2024 21:12

ncdiehl11 requested a review from a team as a code owner September 17, 2024 21:12

ncdiehl11 requested a review from sfoster1 September 17, 2024 21:23

fix liquidName in fixture

c3b4e92

sfoster1 approved these changes Oct 2, 2024

View reviewed changes

sanni-t approved these changes Oct 2, 2024

View reviewed changes

separate behavior params from enable param, add byVolume maps

6a72632

ncdiehl11 requested a review from sanni-t October 8, 2024 16:36

sanni-t reviewed Oct 8, 2024

View reviewed changes

shared-data/liquid-class/schemas/1.json Outdated Show resolved Hide resolved

shared-data/liquid-class/schemas/1.json Outdated Show resolved Hide resolved

shared-data/liquid-class/fixtures/fixture_glycerol50.json Outdated Show resolved Hide resolved

ncdiehl11 added 2 commits October 8, 2024 14:25

refine byVolume properties from arrays to volume-keyed objects

392099d

fix nits

404f9b6

ncdiehl11 requested a review from sanni-t October 8, 2024 18:38

sanni-t approved these changes Oct 8, 2024

View reviewed changes

Merge branch 'edge' into feat_liquid-class-schema

59a6142

sanni-t merged commit 0a3f1a8 into edge Oct 9, 2024
45 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(shared-data): create liquid class schema v1 and fixture #16267

chore(shared-data): create liquid class schema v1 and fixture #16267

ncdiehl11 commented Sep 17, 2024 •

edited

Loading

sfoster1 left a comment

sfoster1 Sep 17, 2024

ncdiehl11 Sep 17, 2024

sfoster1 Sep 17, 2024

codecov bot commented Sep 17, 2024

sfoster1 left a comment

sanni-t left a comment

sanni-t Oct 2, 2024

jbleon95 Oct 3, 2024

sanni-t Oct 3, 2024

sanni-t Oct 2, 2024

sanni-t Oct 2, 2024

sanni-t left a comment •

edited

Loading

sanni-t left a comment

chore(shared-data): create liquid class schema v1 and fixture #16267

chore(shared-data): create liquid class schema v1 and fixture #16267

Conversation

ncdiehl11 commented Sep 17, 2024 • edited Loading

Overview

Test Plan and Hands on Testing

Changelog

Review requests

Risk assessment

sfoster1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 17, 2024

Codecov Report

sfoster1 left a comment

Choose a reason for hiding this comment

sanni-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanni-t left a comment • edited Loading

Choose a reason for hiding this comment

sanni-t left a comment

Choose a reason for hiding this comment

ncdiehl11 commented Sep 17, 2024 •

edited

Loading

sanni-t left a comment •

edited

Loading